Skip to content

Fix an issue if user passes a relative path to Python#26799

Merged
juj merged 3 commits into
emscripten-core:mainfrom
juj:fix_relative_emsdk_python
Apr 29, 2026
Merged

Fix an issue if user passes a relative path to Python#26799
juj merged 3 commits into
emscripten-core:mainfrom
juj:fix_relative_emsdk_python

Conversation

@juj

@juj juj commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Fix an issue if user passes a relative path to Python, and some system cache libraries need rebuilding. Emcc tool would spawn sub-emcc tools to rebuild the cache, with different CWD, resulting in the relative path lookups pointing to the wrong relative directories. To fix the issue, normalize the relative tool paths first.

image

…m cache libraries need rebuilding. Emcc tool would spawn sub-emcc tools to rebuild the cache, with different CWD, resulting in the relative path lookups pointing to the wrong relative directories. To fix the issue, normalize the relative tool paths first.
@juj juj force-pushed the fix_relative_emsdk_python branch from 51fe3c8 to dfb823c Compare April 27, 2026 21:11
@sbc100

sbc100 commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Surely you don't want to be setting EMSDK_PYTHON to a relative path, do you? Otherwise you cannot run it from any directory other than one you are in.

i.e. if you did:

$ EMSDK_PYTHON=../path/to/python
$ emcc
$ cd out
$ emcc  # this one will now fail, right?

i.e. your compile will then only be usable from a single location, even after this change, right?

Another alternative here would be to set EMCC_BATCH_BUILD=0 since the issue here is that the batch builder needs to change the CWD.. which is kind of sad..

@sbc100

sbc100 commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

Another example that simply won't work, even with this change, if you have a relative path like this:

$ make CC=emcc
.. as soon as make tries to run the compiler from a different directory this will fail

Maybe the launcher scripts (currently the only place we reference EMSDK_PYTHON) could warn/assert if its a relative path like this.

@juj

juj commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

My gut impression here was to disable this support, as "this does not make sense, why would you do that?"

But then when I thought about it,

a) Unity has been doing this since forever, where Unity's build system programmatically launches Emscripten. So it crafts the paths on the fly. Other build system might be doing the same, so why actively break them. Sure, it is easy to migrate to absolute paths (which we did), but was equally easy to fix up Emscripten, which this PR shows.

b) It can be useful for ad hoc cmdline runs in testing, e.g. EMSDK_PYTHON ../pythonA/python emcc ... vs EMSDK_PYTHON ../pythonB/python emcc ...

c) fixing this issue is simple - why actively break something that is easy to fix.

Surely you don't want to be setting EMSDK_PYTHON to a relative path, do you? Otherwise you cannot run it from any directory other than one you are in.

Yes, but the above use cases a) (programmatic launching) and b) (ad hoc cmdline use) do not navigate to other directories.

Another example that simply won't work, even with this change, if you have a relative path like this:

Yes, but here the failure will occur when make is launching emcc, not when emcc is internally launching other emcc. It is easier to debug top-level emcc launches from the calling tools, than emcc internally launching other tools.

Maybe the launcher scripts (currently the only place we reference EMSDK_PYTHON) could warn/assert if its a relative path like this.

We could add an assert and say "this is not allowed to work", but since the fix is so simple, I thought it's more straightforward to just fix the root cause?

@sbc100

sbc100 commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Another option is that when we re-execute emcc internally we skip the launcher scripts and use sys.executable emcc.py instead? I think that would fix the problem without introducing the EMSDK_PYTHON into the emscripten source code.

It might also speed up the library builds.

@juj

juj commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

That could work, although it seems conceptually simpler to have a single loop that absolute-pathizes, rather than use sys.executable for python, and then something else for Node. (I presume there could exist a similar issue if a sub-emcc launched by emcc attempted to spawn Node)

@sbc100

sbc100 commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

That could work, although it seems conceptually simpler to have a single loop that absolute-pathizes, rather than use sys.executable for python, and then something else for Node. (I presume there could exist a similar issue if a sub-emcc launched by emcc attempted to spawn Node)

I don't think the latter can happen since since the only sub-compiler commands emcc ever runs are emcc -c (compile only, no node) and emar.

@sbc100

sbc100 commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Note to self: This issue really only exists because of EMCC_BATCH_BUILD=1.. because its only that is the only place where we run in a different CWD.

I don't love EMCC_BATCH_BUILD=1 at all and kind of which we could remove it.. but I understand it makes a big difference on windows?

sbc100 added a commit to sbc100/emscripten that referenced this pull request Apr 28, 2026
Instead move the logic into system_libs.py (the only non-testing place
it is used).

In the test code itself, we just replace the usage with direct calls
to `EMAR`, which is better for decoupling the tests from the compiler
itself.

This is in preparation for a change to skip the wrapper scripts when
the compiler calls itself to build system libs (See emscripten-core#26799)
@juj

juj commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

I don't think the latter can happen since since the only sub-compiler commands emcc ever runs are emcc -c (compile only, no node) and emar.

Ok, updated the implementation to only address Python. I think that's correct, that this won't happen to other sub-tools.

@juj

juj commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

I think the batch build is probably quite a significant speed improvement. Now looking at this, the code area has been refactored by quite a bit since I last looked into this. Though the important bit is that Python multiprocessing is not being used, but we run the subprocess.Popen() items manually.

Comment thread tools/config.py
# reinitialize EMSDK_PYTHON here so that sub-tool spawns will use the same
# Python interpreter as the parent.
if os.environ.get('EMSDK_PYTHON'):
os.environ['EMSDK_PYTHON'] = sys.executable

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! That seems much better.

I also think we could probably localize this to run_build_commands in system_libs.py (since that is the only place its really needed.

But if you want to land as-is and can do some followups.

Comment thread tools/config.py
set_config_from_tool_location('BINARYEN_ROOT', 'wasm-opt', lambda x: os.path.dirname(os.path.dirname(x)))

normalize_config_settings()
normalize_relative_python_path()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of has the wrong name now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah, not sure what's a better name here. Though I'll leave it if you want to address further.

@sbc100 sbc100 Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no worries, feel free to land

@juj juj enabled auto-merge (squash) April 28, 2026 22:06
@sbc100

sbc100 commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

you can ignore the fp6 failures, those are due to a v8 update and will be fixed when llvm changes next roll in.

@juj juj disabled auto-merge April 29, 2026 07:18
@juj juj merged commit 263db4c into emscripten-core:main Apr 29, 2026
27 of 29 checks passed
sbc100 added a commit that referenced this pull request Apr 29, 2026
Instead move the logic into system_libs.py (the only non-testing place
it is used).

In the test code itself, we just replace the usage with direct calls to
`EMAR`, which is better for decoupling the tests from the compiler
itself.

This is in preparation for a change to skip the wrapper scripts when the
compiler calls itself to build system libs (See #26799)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants